Skip to content

Conversation

@felixbarny
Copy link
Member

As discussed in #132566, we'd like to provide private index settings with an IndexSettingsProvider. This is currently not possible because it fails validation for composable index templates (template v2). That's because we first merge all settings from the index template with the ones from the IndexSettingsProviders and then check whether there are private settings.

I've added a way so pass down the settings provided by the IndexSettingsProviders to the validation so that these system-provided settings are allowed to contain private settings.

Another area that needs to be enhanced is to allow downsampling to create indices containing private settings that are copied from the source index. For that reason, I've added a settingsSystemProvided flag to CreateIndexClusterStateUpdateRequest.

@felixbarny felixbarny self-assigned this Aug 29, 2025
@felixbarny felixbarny added >non-issue :Core/Infra/Settings Settings infrastructure and APIs labels Aug 29, 2025
@felixbarny felixbarny marked this pull request as ready for review August 29, 2025 08:00
@elasticsearchmachine elasticsearchmachine added Team:Core/Infra Meta label for core/infra team v9.2.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Aug 29, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@kkrik-es
Copy link
Contributor

The LogsdbIndexModeSettingsProvider already injects private settings:
https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/logsdb/src/main/java/org/elasticsearch/xpack/logsdb/LogsdbIndexModeSettingsProvider.java#L143

I'm probably missing something here..

@felixbarny
Copy link
Member Author

felixbarny commented Aug 29, 2025

Looks like we're skipping adding the private settings during template validation via this hack:

if (MetadataIndexTemplateService.VALIDATE_INDEX_NAME.equals(indexName)) {
// This index name is used when validating component and index templates, we should skip this check in that case.
// (See MetadataIndexTemplateService#validateIndexTemplateV2(...) method)
return MappingHints.EMPTY;
}

I'd rather have the template validation work on the actual settings than the provider behaving differently during validation.

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at this and left a couple of comments (nothing of substance). I keep having a sneaking suspicion that there's a more elegant way to do this, but it might be the late-Friday-afternoon brain telling me that. I'm going to take another look at it next week.

@dakrone dakrone self-requested a review August 29, 2025 21:48
@felixbarny felixbarny requested a review from martijnvg September 1, 2025 15:55
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think with this we can also remove the isTemplateValidation check in LogsdbIndexModeSettingsProvider.

I would like either Ryan or Lee to also take a look at this PR.

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I left two really tiny optional things.

final var combinedSettings = resolveSettings(indexTemplate, projectMetadata.componentTemplates());
// First apply settings sourced from index setting providers:
var finalSettings = Settings.builder();
var additionalSettingsBuilder = Settings.builder();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestion (take it or leave it), perhaps call this systemSettingsBuilder and systemSettings instead of additional… since it gets passed to the validation as the system-provided settings?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTOH, it's coming from the provideAdditionalSettings method and it's being passed in to the validateAdditionalSettings method. I think it make sense as-is. The main thing to convey about these within validateIndexTemplateV2 are that these are additional settings. What's more important about these in the context of validation is that these additional settings are indeed system provided.

Copy link
Contributor

@prdoyle prdoyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like @dakrone I'm a bit worried that we have three different names for what appears to be the same concept:

  • "additional settings"
  • "system provided settings"
  • settings from an IndexSettingProvider

I think we should either use the same name everywhere, or else very clearly define the differences between these concepts.

@felixbarny
Copy link
Member Author

felixbarny commented Sep 11, 2025

Like @dakrone I'm a bit worried that we have three different names for what appears to be the same concept:

  • "additional settings"
  • "system provided settings"
  • settings from an IndexSettingProvider

I think we should either use the same name everywhere, or else very clearly define the differences between these concepts.

The thought process behind it was that CreateIndexClusterStateUpdateRequest and MetadataCreateIndexService#validatePrivateSettingsNotExplicitlySet don't need to know about index setting providers, and I didn't want to restrict the concept of "system-provided settings" to just settings that come from an IndexSettingsProvider, even though that's currently the only source. Renaming the signature

getIndexSettingsValidationErrors(
    final Settings settings,
    @Nullable Settings systemProvided,
    final boolean forbidPrivateIndexSettings
)

to

getIndexSettingsValidationErrors(
    final Settings settings,
    @Nullable Settings additionalSettings,
    final boolean forbidPrivateIndexSettings
)

to match the naming in MetadataIndexTemplateService#validateIndexTemplateV2 seems to decrease clarity. What are these additional settings? IMHO systemProvided is more expressive.

The main concern of MetadataIndexTemplateService#validateIndexTemplateV2 is to collect additionalSettings from IndexSettingsProvider#provideAdditionalSettings, which also get passed in to MetadataCreateIndexService.validateAdditionalSettings. Therefore, renaming additionalSettings to systemProvidedSettings doesn't seem to increase clarity and uniform naming unless we also rename IndexSettingsProvider#provideAdditionalSettings and MetadataCreateIndexService.validateAdditionalSettings which I'm assuming we don't want to do.

What do you think about calling it consistently additionalSettings in the context of MetadataIndexTemplateService (which is where the additional settings get collected) and systemProvided in the context of MetadataCreateIndexService (which is where the validation of private settings is happening).

Personally, I still like it the best as-is. But I'm definitely not married to a particular name and I'm happy to rename to something y'all feel more comfortable with. This doesn't seem like a make or break kind of decision. So let's align on something so that we can move on.

Copy link
Contributor

@prdoyle prdoyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re my naming comment, I'm happy that @felixbarny has thought about it a bunch, and that simplifying the naming isn't straightforward, so let's just merge it as is.

@felixbarny felixbarny merged commit 1b765de into elastic:main Sep 11, 2025
34 checks passed
@felixbarny felixbarny deleted the allow-private-settings-if-system-provided branch September 11, 2025 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Settings Settings infrastructure and APIs external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue Team:Core/Infra Meta label for core/infra team v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants